Skip to content

Keep postRequest primary-chat only#508

Merged
zikajk merged 2 commits into
editor-code-assistant:masterfrom
itkonen:fix/subagent-postrequest-505-upstream
Jun 19, 2026
Merged

Keep postRequest primary-chat only#508
zikajk merged 2 commits into
editor-code-assistant:masterfrom
itkonen:fix/subagent-postrequest-505-upstream

Conversation

@itkonen

@itkonen itkonen commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Issue #505 still remains after the previous fix. The remaining problem is that the postRequest hook is still fired by subagents, which can still cause duplicate or misleading post-request notifications.

This PR limits postRequest exclusively to main/primary-agent chats, and makes subagentPostRequest the exclusive post-request hook for subagent chats.

AI summary

This change separates post-request hook dispatch by chat type:

  • postRequest now runs only after primary-agent prompts.
  • subagentPostRequest now runs only after subagent prompts.
  • subagent hook payloads continue to include parent_chat_id.
  • continue: false, followUp, systemMessage, and exit-2 follow-up behavior continue to work for the selected post-request hook type.

The lifecycle tests, hook documentation, and changelog have been updated to match the new contract.

Refs #505.

Testing

  • clojure -M:test --focus eca.features.hooks-test
  • LC_ALL=C bb test
  • clj-kondo --lint src/eca/features/chat/lifecycle.clj test/eca/features/hooks_test.clj

Checklist

  • I added a entry in changelog under unreleased section.
  • This is not an AI slop.

Route subagent completions exclusively through subagentPostRequest so
post-request hooks do not emit duplicate notifications for subagent turns.
Update the hook docs, changelog, and lifecycle tests to match the contract.

🤖 Generated with [ECA](https://eca.dev) (openai/gpt-5.5 - high)

Co-Authored-By: eca-agent <git@eca.dev>
@itkonen

itkonen commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

CI failure is pre-existing and unrelated to this PR

The background-job-lifecycle integration test failure is caused by a race condition in src/eca/features/background_tasks.clj — nothing touched by this PR.

Root cause: kill-job! calls kill-job-impl (sends SIGTERM) and then atomically sets :status :killed. The monitor-process-exit! daemon thread can race ahead: it dereferences the now-exited process, gets exit code 143, and calls update-job-on-exit before the kill status has been committed — overwriting it with :failed instead.

Fix: swap the order in kill-job! — set :status :killed in the registry before calling kill-job-impl, so the monitor thread always sees :killed and the guard in update-job-on-exit correctly skips the overwrite:

(defn kill-job! [job-id]
  (if-let [job (get-job job-id)]
    (if (= :running (:status job))
      (do
        ;; Commit :killed status before destroying the process so that
        ;; monitor-process-exit! never overwrites it with :failed (exit 143).
        (swap! registry* (fn [reg]
                           (-> reg
                               (assoc-in [:jobs job-id :status] :killed)
                               (assoc-in [:jobs job-id :ended-at] (Instant/now)))))
        (kill-job-impl job)
        (logger/info logger-tag "Killed background job" {:id job-id})
        true)
      ...)))

The existing unit test "does not overwrite :killed status" in background_tasks_test.clj already covers this scenario.

@ericdallo

Copy link
Copy Markdown
Member

c/c @zikajk

@zikajk

zikajk commented Jun 19, 2026

Copy link
Copy Markdown
Member

@itkonen @ericdallo
I said it was a feature, but it isn't. I got confused by some plugins, but this is actually a good change. Thanks :-).

@zikajk zikajk enabled auto-merge June 19, 2026 20:09
@zikajk zikajk disabled auto-merge June 19, 2026 20:13
@zikajk zikajk merged commit e62cd4c into editor-code-assistant:master Jun 19, 2026
9 checks passed
@itkonen

itkonen commented Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Great this resolved! I already started orchestrating a fix around your earlier idea around parent_chat_id, which would have also worked fine. :) But I guess this first solution makes it simpler to separate main and sub-agent events if there are no compatibility issues.

@zikajk

zikajk commented Jun 20, 2026

Copy link
Copy Markdown
Member

@itkonen I was wrong, because Claude Code allows you to specify hooks within subagents frontmatter, and when you define a Stop hook there, it automatically changes to a subagentStop hook. But in all other cases (hooks in the global / project config), the Stop and subagentStop hooks are separate :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants